Don't populate excluded fields while populating existing models history #381

uadnan · 2018-04-29T19:18:20Z

Consider existing model, on which HistoricalRecords is added with some fields excluded,

 class MyModel(models.Model):
    name = models.CharField(max_length=100)
    created = models.DateTimeField(auto_now_add=True)
    modified = models.DateTimeField(auto_now=True)

    history = HistoricalRecords(excluded_fields=['created', 'modified'])

And here in this case, populate_history will still try to instantiate the model with created and modified value, that leads to this error:

TypeError: 'created' is an invalid keyword argument for this function

This pull request address above described issue and replaces #304

codecov-io · 2018-04-29T19:36:20Z

Codecov Report

Merging #381 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #381      +/-   ##
==========================================
+ Coverage   97.52%   97.54%   +0.02%     
==========================================
  Files          15       15              
  Lines         605      610       +5     
  Branches       78       80       +2     
==========================================
+ Hits          590      595       +5     
  Misses          9        9              
  Partials        6        6

Impacted Files	Coverage Δ
...ple_history/management/commands/_populate_utils.py	`100% <ø> (ø)`	⬆️
simple_history/models.py	`98.59% <100%> (+0.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 596b75c...316836b. Read the comment docs.

rossmechanic · 2018-05-11T20:30:20Z

simple_history/management/commands/_populate_utils.py

    historical_instances = [
        history_model(
            history_date=getattr(instance, '_history_date', now()),
            history_user=getattr(instance, '_history_user', None),
            **{
                field.attname: getattr(instance, field.attname)
                for field in instance._meta.fields
+                if not excluded_fields & {field.name, field.attname}


Why use field.attname here? In fields_included we only check on field.name: https://github.com/treyhunner/django-simple-history/blob/master/simple_history/models.py#L142

@rossmechanic Ok, I've updated the code to only have check on field.name

uadnan · 2018-05-23T17:16:18Z

@rossmechanic would like to review this PR again?

rossmechanic · 2018-05-29T22:25:00Z

Yes going to review this week! @uadnan

rossmechanic · 2018-06-04T20:47:41Z

simple_history/models.py

-        attrs = {'__module__': self.module}
+        attrs = {
+            '__module__': self.module,
+            'excluded_fields': self.excluded_fields


This prevents someone from using excluded_fields as a field_name, no?

Or it will get overwritten by that field name, but either way it's not safe.

Could we add it to the Meta class of the historical model instead?

If that moved to meta class that will lead to exception by Django. If that fixed some how that will not be detected by Django migrator and lead to excluded fields being ignored within Django Migrations.

I'm adding a _ prefix to excluded_fields thus user can have a field with that name.

rossmechanic · 2018-06-14T16:00:45Z

@uadnan any update?

kseever · 2018-06-14T16:24:34Z

simple_history/tests/tests/test_commands.py

+        models.PollWithExcludeFields.history.all().delete()
+        management.call_command(self.command_name,
+                                'tests.pollwithexcludefields', auto=True)
+        update_record = models.PollWithExcludeFields.history.all()[0]


update_record seems a bit misleading since no update actually happened. How about initial_history_record?

- tests the management command on an historical record which has excluded fields - tests the excluded fields are retrieved from the current model instance

Previously, it was pre-loading directly related objects.

excluded_fields -> _history_excluded_fields update_record -> initial_history_record

uadnan · 2018-06-14T17:51:56Z

@rossmechanic Can you please have a look again?

rossmechanic · 2018-06-14T17:52:40Z

CHANGES.rst

@@ -1,13 +1,16 @@
 Changes
 =======

+Unreleased
+----------
+- Fix TypeError on populate_history if excluded_fields are specified


Want to add the PR number to this? (gh-381)

rossmechanic · 2018-06-14T19:11:31Z

simple_history/models.py

-        attrs = {'__module__': self.module}
+        attrs = {
+            '__module__': self.module,
+            '_history_excluded_fields': self.excluded_fields


Still don't love adding this on the model itself. Will think about this about and think about any cleaner solutions. Going to be merging some bulk_history_create updates anyway so this will need to be rebased. I'll let you know if I can think of any other way of doing this. Otherwise we'll go with this.

For reference, this is why we can't put excluded_fields on the meta class. So I'm fine with the current implementation.

rossmechanic · 2018-06-14T21:12:00Z

simple_history/tests/tests/test_commands.py

+        poll = models.PollWithExcludeFields.objects.create(
+            question="Will this work?", pub_date=datetime.now())
+        models.PollWithExcludeFields.history.all().delete()
+        management.call_command(self.command_name,


add stderr=StringIO() and stdout=StringIO() here so the output isn't printed in the console when running tests

rossmechanic · 2018-06-14T21:13:43Z

@uadnan so I thought about this – Happy with this approach to the problem. Just need to rebase and make sure your test in test_commands.py doesn't print to the console. Then this is all good and I'm happy to merge. Let me know if you don't have time to rebase – If so, I can rebase it and merge it. Trying to get this in ASAP so I can do a micro release with the fix today.

rossmechanic · 2018-06-15T14:28:48Z

@uadnan I rebased this in #410. So going to close this PR and merge that one. Thanks for this fix!

rossmechanic reviewed May 11, 2018

View reviewed changes

rossmechanic mentioned this pull request Jun 1, 2018

Problem to use history = HistoricalRecords(excluded_fields=['description']) #402

Closed

rossmechanic reviewed Jun 4, 2018

View reviewed changes

kseever reviewed Jun 14, 2018

View reviewed changes

uadnan and others added 7 commits June 14, 2018 22:44

Don't populate excluded fields while populating existing model history

34f5ef2

Adds tests for two excluded-fields fixes:

01e346d

- tests the management command on an historical record which has excluded fields - tests the excluded fields are retrieved from the current model instance

Adds test for foreign-key excluded field.

a8b1b94

Reduces number of queries made by History-Model.get_instance.

c1d3ddc

Previously, it was pre-loading directly related objects.

Adjustments

f3d21d7

Updated AUTHORS and log CHANGES

5937337

Renames

316836b

excluded_fields -> _history_excluded_fields update_record -> initial_history_record

rossmechanic reviewed Jun 14, 2018

View reviewed changes

rossmechanic mentioned this pull request Jun 14, 2018

Allow alternative user model for tracking history_user #371

Merged

rossmechanic pushed a commit that referenced this pull request Jun 15, 2018

Rebased #381

86e8445

rossmechanic mentioned this pull request Jun 15, 2018

Don't populate excluded fields in populate history #410

Merged

9 tasks

rossmechanic closed this Jun 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't populate excluded fields while populating existing models history #381

Don't populate excluded fields while populating existing models history #381

uadnan commented Apr 29, 2018 •

edited

Loading

codecov-io commented Apr 29, 2018 •

edited

Loading

rossmechanic May 11, 2018

uadnan May 14, 2018

uadnan commented May 23, 2018

rossmechanic commented May 29, 2018

rossmechanic Jun 4, 2018

rossmechanic Jun 4, 2018

rossmechanic Jun 4, 2018

uadnan Jun 14, 2018

rossmechanic commented Jun 14, 2018

kseever Jun 14, 2018

uadnan commented Jun 14, 2018

rossmechanic Jun 14, 2018

rossmechanic Jun 14, 2018

rossmechanic Jun 14, 2018

rossmechanic Jun 14, 2018

rossmechanic commented Jun 14, 2018

rossmechanic commented Jun 15, 2018

Don't populate excluded fields while populating existing models history #381

Don't populate excluded fields while populating existing models history #381

Conversation

uadnan commented Apr 29, 2018 • edited Loading

codecov-io commented Apr 29, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uadnan commented May 23, 2018

rossmechanic commented May 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossmechanic commented Jun 14, 2018

Choose a reason for hiding this comment

uadnan commented Jun 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossmechanic commented Jun 14, 2018

rossmechanic commented Jun 15, 2018

uadnan commented Apr 29, 2018 •

edited

Loading

codecov-io commented Apr 29, 2018 •

edited

Loading